Making UDS error handling and recovery more robust. #189

dhermes · 2020-09-25T19:00:08Z

Fixes #128.

dhermes · 2020-09-25T19:17:01Z

Some notes:

I've been using https://github.com/dhermes/local-dd-agent to start and stop a local dd-agent
I fully expect the maintainers to want more quality control on the (fairly large) change set I dropped from thin air. Please feel free to pick it apart.
I hope the commits have broken it up in a way that's easy to digest.

dhermes · 2020-09-25T19:17:28Z

lib/statsd.js

+  });
+}
+
+function trySetNewSocket(client) {


I'm very open to a naming convention other try in trySetNewSocket()

dhermes · 2020-09-25T19:17:42Z

lib/statsd.js

+  }
+}
+
+function maybeAddUDSErrorHandler(client, lastSocketCreateTime) {


I'm very open to a naming convention other than maybe in maybeAddUDSErrorHandler()

I don't get too hung up on names, unless I think it doesn't fit the style. Not too much of a naming style in here so it's fine. :) Spent a bunch of time making sure I understood the new paths in here, for this function and others. I didn't see any issues, looks good to me.

dhermes · 2020-09-25T19:18:17Z

lib/statsd.js

@@ -526,3 +502,59 @@ Client.prototype.childClient = function (options) {

 exports = module.exports = Client;
 exports.StatsD = Client;
+
+function udsErrorHandler(client, lastSocketCreateTime) {


I'm not sure if you want these handlers here at the end

I'm not sure if you'd prefer client be this and we just put these helpers on the Client prototype

I think it's fine as is. Great to see the nice docs there.

dhermes · 2020-09-25T19:18:53Z

lib/statsd.js

@@ -354,7 +321,16 @@ Client.prototype.sendMessage = function (message, callback) {
    return;
  }

-  if (!this.socket) {
+  const socketWasMissing = !this.socket;


It may be considered a breaking change that I've added an attempt to recover here

Good point, thanks. Not sure yet if this will be a minor or major release, but I think this part by itself would fit in ok with what has been minor releases.

dhermes · 2020-09-25T19:19:08Z

lib/statsd.js

@@ -48,6 +51,8 @@ const Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
  this.cacheDnsTtl = options.cacheDnsTtl || CACHE_DNS_TTL_DEFAULT;
  this.host = options.host || process.env.DD_AGENT_HOST;
  this.port = options.port || parseInt(process.env.DD_DOGSTATSD_PORT, 10) || 8125;
+  this.path = options.path;


I've added four public fields to Client (path, stream, udsGracefulRestartRateLimit and isChild), this may not be a desirable addition to the "exported" interface

I haven't updated the TypeScript definition(s) to reflect new exports / fields

That's fine, and you shouldn't add these to TypeScript definitions, others ones aren't mentioned in there like this (I don't think)

dhermes · 2020-09-25T20:56:53Z

To verify this change I've been running sent-to-uds.js

const process = require("process");

const hotShots = require("./index");

const DATADOG_ADDRESS = process.env.DATADOG_ADDRESS;

function toPath(datadogAddress) {
  if (datadogAddress.startsWith("unix://")) {
    // NOTE: The length of the "unix://" prefix is 7.
    return datadogAddress.slice(7, datadogAddress.length);
  }

  return datadogAddress;
}

function createUDSClient() {
  const opts = {
    mock: false,
    maxBufferSize: 0,
    path: toPath(DATADOG_ADDRESS),
    protocol: "uds",
    udsGracefulErrorHandling: true,
    udsGracefulRestartRateLimit: 1000,
  };
  return new hotShots.StatsD(opts);
}

function makeEventLoop(udsClient) {
  return function compute() {
    console.log(
      JSON.stringify({
        message: "Doing very big computation",
        level: "info",
        _timestamp: new Date(),
      }),
    );
    udsClient.increment("computation.uds", 1, 1, []);
  };
}

async function main() {
  const udsClient = createUDSClient();
  const compute = makeEventLoop(udsClient);
  setInterval(compute, 500);
}

if (require.main === module) {
  main();
}

via

SOCKET_DIR="${HOME}/var-run-datadog"
DATADOG_ADDRESS="unix://${SOCKET_DIR}/dsd.socket" node ./sent-to-uds.js

and then turning on and off the local-dd-agent (https://github.com/dhermes/local-dd-agent) via

make run DATADOG_ADDRESS="${SOCKET_DIR}/dsd.socket"

bdeitte · 2020-09-26T01:53:02Z

@dhermes Thanks for the PR and all of the info! Neat to see local-dd-agent too. I see the tests are still failing in here, looks like something isn't going right on the Mac version. Also as noted in #188 looks like we have some overlap here, haven't had a chance yet myself to look over everything and understand.

dhermes · 2020-09-26T01:59:02Z

Yup; I haven't had a chance to parse the test failures but will attempt to tackle it next week. I didn't want to devote too much time building a cathedral before getting sign off from maintainers.

Once / if you think this looks promising I can start to tackle the test failures? I'd love an assist if the Travis CI failures look "easy" to tackle for you (but by all means not needed).

Also, since there are 4 distinct issues, I can send 4 different PRs if you'd rather (the commits themselves in this PR can be broken up nicely to fix each of the 4 issues separately).

bdeitte · 2020-09-27T23:47:23Z

lib/constants.js

+  if (process.platform === 'linux') {
+    return [
+      os.constants.errno.ENOTCONN,
+      os.constants.errno.ECONNREFUSED,


Huh, I didn't realize there was a constant here available on Node 8 and above. Good to know.

bdeitte · 2020-09-27T23:55:15Z

I took a pretty quick pass on this, but great to see all the work done and makes general sense to me. When the tests are passing (and I have a little more time) I'll take another look at this and see if I have any more feedback. This UDS handling has been looking ugly for awhile (from what I can gather from the bug reports), glad to see someone take it all on and make it work better.

I may be able to help with the tests here at some point (do run a Mac here) if you need it, just been short on time recently. It's not clear to me right away what's going wrong in there- oddly enough it's a mix of all sorts of different tests failing in there (https://travis-ci.org/github/brightcove/hot-shots/jobs/730370815#L1518).

…` in constructor. Also using these in places where `options` was used.

This way we can retry using this helper.

This is in advance of a change that will allow these to be OS specific.

See https://nodejs.org/docs/latest-v12.x/api/errors.html for an example (in `error.errno`) explaining why error codes are negative.

…andler()`.

This way `this.socket` is only replaced if `createTransport()` returned an actual socket (vs. `null`).

See: https://travis-ci.org/github/brightcove/hot-shots/jobs/730354851 This was due to `eslint` failures. Also changed some `==` to `===` and added / removed some `;` in lines that needed / didn't need.

This "bug" was introduced to my bugfix in a re-factor.

dhermes · 2020-09-30T13:12:14Z

@bdeitte Tests are green here so should be good to review.

bdeitte · 2020-10-01T02:12:06Z

Awesome. I spent some time with tonight and don't see any issues. Thanks again for this! Going to release in a minor release here in a bit.

dhermes commented Sep 25, 2020

View reviewed changes

bdeitte reviewed Sep 27, 2020

View reviewed changes

prognant mentioned this pull request Sep 28, 2020

Fix uds reconnection logic #188

Closed

Danny Hermes added 17 commits September 28, 2020 09:40

Storing path, stream and udsGracefulRestartRateLimit on `client…

8d7fe2b

…` in constructor. Also using these in places where `options` was used.

Getting rid of socketCreateLimit local (not needed any longer).

5eb2e19

Introduce trySetNewSocket() helper.

700fb83

This way we can retry using this helper.

Replacing switch statement with UDS_ERROR_CODES constant.

ccc83e4

This is in advance of a change that will allow these to be OS specific.

Using -code for check within UDS_ERROR_CODES.

eef577c

See https://nodejs.org/docs/latest-v12.x/api/errors.html for an example (in `error.errno`) explaining why error codes are negative.

Using OS-specific error codes in udsErrors().

9db6aa0

Factoring out udsErrorHandler() helper.

46da650

Factoring out addUDSErrorHandler() helper.

5d194ab

Keeping around client.isChild so it can be re-used.

a8845b2

Removing spurious empty line.

23b6aaa

Adding UDS recovery to Client.prototype.sendMessage.

572cb0c

Checking isChild / udsGracefulErrorHandling in `maybeAddUDSErrorH…

0fc3a68

…andler()`.

Modifying udsErrorHandler() to check truthy socket.

7b36227

This way `this.socket` is only replaced if `createTransport()` returned an actual socket (vs. `null`).

Remove spurious space between ! and var.

7cf8ad8

Adding explicit import for process.

1e364f5

Adding JSDoc to newly added code.

15e9eb1

See: https://travis-ci.org/github/brightcove/hot-shots/jobs/730354851 This was due to `eslint` failures. Also changed some `==` to `===` and added / removed some `;` in lines that needed / didn't need.

Fixing this -> client reference.

f3d3fd6

This "bug" was introduced to my bugfix in a re-factor.

dhermes force-pushed the issue-128 branch from 6837af7 to c950dd2 Compare September 28, 2020 14:59

Changing unit test hardcoded references to 107/111.

884c980

dhermes force-pushed the issue-128 branch from c950dd2 to 884c980 Compare September 28, 2020 15:07

bdeitte merged commit 907dd57 into brightcove:master Oct 1, 2020

chotiwat mentioned this pull request Dec 4, 2020

Handle UDS errors occurring when sending metrics #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making UDS error handling and recovery more robust. #189

Making UDS error handling and recovery more robust. #189

dhermes commented Sep 25, 2020

dhermes commented Sep 25, 2020 •

edited

Loading

dhermes Sep 25, 2020

dhermes Sep 25, 2020

bdeitte Oct 1, 2020

dhermes Sep 25, 2020

bdeitte Oct 1, 2020

dhermes Sep 25, 2020

bdeitte Oct 1, 2020

dhermes Sep 25, 2020

bdeitte Sep 27, 2020

dhermes commented Sep 25, 2020

bdeitte commented Sep 26, 2020

dhermes commented Sep 26, 2020

bdeitte Sep 27, 2020

bdeitte commented Sep 27, 2020

dhermes commented Sep 30, 2020

bdeitte commented Oct 1, 2020

Making UDS error handling and recovery more robust. #189

Making UDS error handling and recovery more robust. #189

Conversation

dhermes commented Sep 25, 2020

dhermes commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhermes commented Sep 25, 2020

bdeitte commented Sep 26, 2020

dhermes commented Sep 26, 2020

Choose a reason for hiding this comment

bdeitte commented Sep 27, 2020

dhermes commented Sep 30, 2020

bdeitte commented Oct 1, 2020

dhermes commented Sep 25, 2020 •

edited

Loading